-
Notifications
You must be signed in to change notification settings - Fork 104
[sql-41] firewalldb: add migration code for kvstores from kvdb to SQL #1079
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
[sql-41] firewalldb: add migration code for kvstores from kvdb to SQL #1079
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think this doesnt need to be based on the sessions pr right? yes order of merge matters, but i think we dont need to base this one off of that one or at least can change the base branch here so that we can review both migrations in parallel
4151b93
to
7a517ad
Compare
Thanks for the swift review @ellemouton 🙏! I've rebased this to be based on master and not the sessions migration PR. Note that I've kept 2 commits from the sessions migration PR in this PR (the 2 first), as they are required. So feel free to ignore reviewing the first to commits of this PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looking great! a couple of comments. I'll do a final round once the session PR is in 👍
firewalldb/interface.go
Outdated
@@ -141,4 +141,7 @@ type FirewallDBs interface { | |||
RulesDB | |||
PrivacyMapper | |||
ActionDB | |||
|
|||
// Close closes the underlying store. | |||
Close() error |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dont think we need this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just call t.Cleanup sooner
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(again commit message could explain more as it took me a sec to figure out why you're adding this)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks 🙏! Addressed with a new commit in the latest push :)
@@ -14,29 +14,21 @@ var ( | |||
ErrNoSuchKeyFound = fmt.Errorf("no such key found") | |||
) | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
your commit messages could be fleshed out a bit more pls. it isnt clear why this needs to be exported (same for other commits in this PR)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, completely forgot updating the commit messages in the latest push. I've updated them in the latest push though, so I hope the commits should be understandable now!
firewalldb/sql_migration.go
Outdated
// SQL database during the migration. | ||
var allParams []kvParams | ||
|
||
err := kvStore.Update(func(kvTx *bbolt.Tx) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why Update
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was due to the migration utilising the getMainBucket
helper function, which with the previous version created the main bucket if it didn't exist. However I realised that if the bucket doesn't exist, it just means that we can skip migrating the records, so I updated the PR to do that instead with the latest push.
firewalldb/sql_migration.go
Outdated
return err | ||
} | ||
|
||
// After the migration is done, we validate that all inserted kvParams |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cool yeah thanks for separating this 👍 i think it is especially important for the kv stores since there is a higher chance of overwriting 👍
firewalldb/sql_migration_test.go
Outdated
}, | ||
{ | ||
name: "random records", | ||
populateDB: randomKVRecords, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think it's worth having a case that does some explicit entries that write to all the store types. currently we only do that for the random one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea! Added an extra test with the latest push.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it still doesnt have a deterministic test that stores in 2 feature tables/ 2 rule tables etc right?
like, currently globalRecords
only inserts under a single rule name, for example
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also, looks like for this new test, all the key-value pairs inserted at all the levels are the same... so again it doesnt properly test that the correct things mapping is taking place
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yeah, good point! I updated this test quite a bit (now renamed to allEntryCombinations
) to hopefully cover all different types of entries in a deterministic way. I think it should now cover all of those cases.
7a517ad
to
10550d4
Compare
Thanks for the review @ellemouton 🙏! Addressed the latest feedback and rebased this once again on the important commits from #1051. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work 🚀, took an initial look, the structure looks great!
firewalldb/kvstores_kvdb.go
Outdated
rules -> perm -> rule-name -> global -> {k:v} | ||
-> sessions -> group ID -> session-kv-store -> {k:v} | ||
-> feature-kv-stores -> feature-name -> {k:v} | ||
"rules" -> "perm" -> rule-name -> "global" -> {k:v} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for the update here 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah thanks for the fix!! 🙏
firewalldb/sql_migration.go
Outdated
// It inserts the records into the SQL database and asserts that | ||
// the migrated values match the original values in the KV store. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: there are no assertions in here, same in processGroupBucket
firewalldb/sql_migration.go
Outdated
ruleSqlId int64, groupAlias []byte, | ||
groupBucket *bbolt.Bucket) ([]kvParams, error) { | ||
|
||
groupSqlId, err := sqlTx.GetSessionIDByAlias( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: contract to line
firewalldb/sql_migration.go
Outdated
ruleNameBucket := mainBucket.Bucket(k) | ||
if ruleNameBucket == nil { | ||
return fmt.Errorf("rule bucket %s "+ | ||
"not found", string(k)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you don't need those casts as the %s directive already does it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah nice, thanks 🙏!
firewalldb/sql_migration_test.go
Outdated
Global bool | ||
GroupID *session.ID |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you don't use the nilness of GroupID, right? could you use fn.Option[GroupID]
to encode that this is a global entry?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea, added :)
firewalldb/kvstores_kvdb.go
Outdated
-> sessions -> group ID -> session-kv-store -> {k:v} | ||
-> feature-kv-stores -> feature-name -> {k:v} | ||
"rules" -> "perm" -> rule-name -> "global" -> {k:v} | ||
"session-kv-store" -> group ID -> {k:v} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing the ->
here
firewalldb/kvstores_kvdb.go
Outdated
rules -> perm -> rule-name -> global -> {k:v} | ||
-> sessions -> group ID -> session-kv-store -> {k:v} | ||
-> feature-kv-stores -> feature-name -> {k:v} | ||
"rules" -> "perm" -> rule-name -> "global" -> {k:v} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
while we are here, can we add <
& >
around keys that vary? like <rule-name>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and <group ID>
& <feature name>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea! done :)
firewalldb/kvstores_kvdb.go
Outdated
rules -> perm -> rule-name -> global -> {k:v} | ||
-> sessions -> group ID -> session-kv-store -> {k:v} | ||
-> feature-kv-stores -> feature-name -> {k:v} | ||
"rules" -> "perm" -> rule-name -> "global" -> {k:v} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah thanks for the fix!! 🙏
firewalldb/sql_migration.go
Outdated
continue | ||
} | ||
|
||
err = mainBucket.ForEach(func(k, v []byte) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggest adding a short comment: "Loop over each rule-name in the bucket"
firewalldb/sql_migration_test.go
Outdated
tempKvRecord := kvStoreRecord{ | ||
RuleName: ruleName, | ||
GroupID: &sess.GroupID, | ||
FeatureName: featureNameOpt, | ||
EntryKey: entryKey, | ||
Value: entryVal, | ||
Perm: false, | ||
Global: global, | ||
} | ||
|
||
insertKvRecord(t, ctx, boltDB, tempKvRecord) | ||
|
||
permKvRecord := kvStoreRecord{ | ||
RuleName: ruleName, | ||
GroupID: &sess.GroupID, | ||
FeatureName: featureNameOpt, | ||
EntryKey: entryKey, | ||
Value: entryVal, | ||
Perm: true, | ||
Global: global, | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you are inserting the same key-value pair in both, then things are not properly being tested since you are not testing that the one namespace doesnt overwrite the other
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not fully sure that I got exactly what this feedback was referring to here, but I added additional inserts to the allEntryCombinations
as mentioned in #1079 (comment). I hope that covers this feedback, else let me know!
firewalldb/sql_migration_test.go
Outdated
}, | ||
{ | ||
name: "random records", | ||
populateDB: randomKVRecords, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also, looks like for this new test, all the key-value pairs inserted at all the levels are the same... so again it doesnt properly test that the correct things mapping is taking place
firewalldb/sql_migration_test.go
Outdated
// Create a session that we can reference. | ||
sess, err := sessionStore.NewSession( | ||
ctx, "test", session.TypeAutopilot, | ||
time.Unix(1000, 0), "something", | ||
) | ||
require.NoError(t, err) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
only need to create session if !global
@ViktorTigerstrom, remember to re-request review from reviewers when ready |
10550d4
to
7bb668e
Compare
Thanks a lot for the reviews and the great suggestions @ellemouton & @bitromortac 🙏🎉!!! I've overhauled this PR quite a bit since the last push to address that feedback just FYI, so expect the PR to contain quite a lot of new changes. Also, apologies for the linting error. I'll address that one after the next review round, in order to not waste extra CI resources just for that. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice changes 🔥! I think the general structure looks good, I have a suggestion that we could make our store consistent such that if []byte{}
is passed in we would receive a []byte{}
. If we would need to be able to store nil
we could remove the NOT NULL
constraint, but I'm not sure it's needed. If this works, we could remove uncertainty and test code.
db/sqlc/queries/kvstores.sql
Outdated
AND feature_id IS NULL; | ||
|
||
-- name: GetSessionKVStoreRecord :one | ||
-- name: GetSessionGroupKVStoreRecord :one |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: we could shorten SessionGroup
to just Group
as I think it isn't that ambiguous
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed :)
firewalldb/kvstores_kvdb.go
Outdated
-> feature-kv-stores -> feature-name -> {k:v} | ||
"rules" -> "perm" -> <rule-name> -> "global" -> {k:v} | ||
-> "session-kv-store" -> <group-ID> -> {k:v} | ||
-> "feature-kv-stores" -> <feature-name> -> {k:v} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tiny nit: I would prefer to use spaces only instead of tabs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ViktorTigerstrom - you should be able to set your IDE up to auto replace tabs as spaces
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
|
||
// GroupID is the legacy session group alias that the entry is | ||
// associated with. For global entries, this will be fn.None[[]byte]. | ||
groupAlias fn.Option[[]byte] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: doc mismatch, same in sqlKvEntry
|
||
var pairs []*kvEntry | ||
|
||
// 1) Collect all key-value pairs from the KV store. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't anticipate any memory concerns here I guess since value objects seem to be quite small right? I think for actions we may need to consider a chunked approach
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah this is the sort of thing we should write benchmarks for. perhaps a good candidate for a follow up: vitkor, you can create a TODO in the epic description & then it can be done in a follow up i think (for all the migrations)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea with the benchmarking! Added that to the epic.
We don't anticipate any memory concerns here I guess since value objects seem to be quite small right? I think for actions we may need to consider a chunked approach
Yeah agree, we discussed this a bit offline previously, and for kvstores we deemed that this shouldn't be an issue as there shouldn't be much data stored in the kvstores for our users. But definitely agree that this becomes much more important for the actions migration, and I'd argue that it might be worth it to insert and validate the migration of one action at a time there just to ensure that this can't be an issue there either.
// Compare the value of the migrated entry with the original | ||
// value from the KV store. | ||
// NOTE: if the insert a []byte{} value into the sqldb as the | ||
// entry value, and then retrieve it, the value will be | ||
// returned as nil. The bytes.Equal will pass in that case, | ||
// and therefore such cases won't error out. The kvdb instance | ||
// can store []byte{} values. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd favor the store to return []byte{}
if it internally stores it as nil
when passing in []byte{}
, with a conversion at the lowest layer. Can the kv database currently have []byte(nil)
stored via some code path? If that's the case we may want to remove NOT NULL
from value BLOB
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can the kv database currently have []byte(nil) stored via some code path? If
Should make sure to cover in a unit test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd favor the store to return []byte{} if it internally stores it as nil when passing in []byte{}
I agree that this would be the ideal solution, but I've tested and unfortunately I can't get the SQLLite DB to return it as a []byte{}
, even when removing the NOT NULL
constraint. For Postgres DBs, I can get it to work. So I believe this is a differentiation of how the DB implementations handles that type of data, and therefore IMO the bytes.equals
solution is the best we can do.
Can the kv database currently have []byte(nil) stored via some code path?
In a prod env, there should be no code path that leads to a []byte(nil)
value being inserted from my research.
Should make sure to cover in a unit test
The allEntryCombinations
& randomKVEntries
unit tests already covered that by inserting nil
values, which are equivalent, but just to explicitly also insert a value set to exactly []byte(nil)
, I added an extra nilSliceValue = []byte(nil)
to the allEntryCombinations
unit test :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't get the SQLLite DB to return it as a []byte{}
Right, I tested that as well, and it doesn't seem to work. I meant another solution, to update sqlKVStore.Get
to convert []byte(nil)
to []byte{}
, I think that could resolve this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean that we'd implement a conversion in the actual sql store layer (the SQLDB
struct, i.e. the layer between the SQLQueries
and the rest of the application), which converts nil
values fetched from the db to []byte{}
?
We could potentially get that to work in the sql_migration_test.go
file, but not in the sql_migration.go
file, as we need to use the pure SQLQueries
object in the sql_migration.go
file (this becomes very important when implementing migrations using the sqldb/v2
library). Eventually I think we may want to go for the using the pure SQLQueries
object in the test assertions as well, but I think that's out of scope for this PR.
Potentially we could manually update the query implementation to always return []byte{}
on nil
values, but that would mean that we wouldn't be able to use sqlc
to generate this query, which I'm not sure we'd want to do due to the downsides? Would be appreciate your opinions on that as well :)!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, one workaround could be to apply a patch with gen_sqlc_docker.sh
like here to the respective queries.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm yeah cool idea! I can look into if it'd be possible to replace nil
results with []byte{}
for all queries in the sql db by a adding some kind of script there, if you think it's worth it. Do you think we should do that in this PR, or as a follow-up?
firewalldb/sql_migration_test.go
Outdated
getRuleID := func(ruleName string) int64 { | ||
ruleID, ok := ruleIDs[ruleName] | ||
if !ok { | ||
ruleID, err = sqlStore.GetRuleID( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: contract
firewalldb/sql_migration_test.go
Outdated
// insertTempAndPermEntry populates the kv store with one entry for the local | ||
// temp store, and one entry for the local store. Both of the entries will be | ||
// inserted with the same groupAlias, ruleName, entryKey and entryValue. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: this acts on global and local stores, so I think local
could be removed
firewalldb/sql_migration_test.go
Outdated
|
||
// First lets create standard entries at all levels, which represents | ||
// the entries added by other tests. | ||
gEntries1 := globalEntries(t, ctx, boltDB, sessionStore) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I would favor the following pattern, as it makes the test more robust with respect to changes
var result []*kvEntry
add := func(entry []*kvEntry) {
result = append(result, entry...)
}
add(globalEntries(t, ctx, boltDB, sessionStore))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah nice, agree :). Addressed!
firewalldb/sql_migration_test.go
Outdated
ctx, "initial-session", session.Type(uint8(rand.Intn(5))), | ||
time.Unix(1000, 0), randomString(rand.Intn(10)+1), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I suggest to remove randomness here, since we are not testing session functionality, to have clearer code
} else { | ||
// Else generate a random value for all entries, | ||
entry.value = []byte(randomString(rand.Intn(100) + 1)) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this was already done above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great work!
I think g2g on next push 🛩️
firewalldb/kvstores_kvdb.go
Outdated
-> feature-kv-stores -> feature-name -> {k:v} | ||
"rules" -> "perm" -> <rule-name> -> "global" -> {k:v} | ||
-> "session-kv-store" -> <group-ID> -> {k:v} | ||
-> "feature-kv-stores" -> <feature-name> -> {k:v} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ViktorTigerstrom - you should be able to set your IDE up to auto replace tabs as spaces
firewalldb/sql_migration_test.go
Outdated
var store KVStore | ||
switch { | ||
case entry.groupAlias.IsNone() && entry.perm: | ||
store = tx.Global() | ||
case entry.groupAlias.IsNone() && !entry.perm: | ||
store = tx.GlobalTemp() | ||
case entry.groupAlias.IsSome() && !entry.perm: | ||
store = tx.LocalTemp() | ||
case entry.groupAlias.IsSome() && entry.perm: | ||
store = tx.Local() | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note that in the example i posted before, we could not possibly end up with a nil store. So if you want to have these 4 cases and not pre assigne a default, the rather have a default case to catch that none of the cases match
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok I changed to your previous example :). The reason I went for the above, was that I liked that it was very explicit what each possible logical combination would lead to, and that this was in a test file (which is why I avoided the extra unreachable default
case).
But addressed this with your previous suggestion :)
|
||
var pairs []*kvEntry | ||
|
||
// 1) Collect all key-value pairs from the KV store. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah this is the sort of thing we should write benchmarks for. perhaps a good candidate for a follow up: vitkor, you can create a TODO in the epic description & then it can be done in a follow up i think (for all the migrations)
// Compare the value of the migrated entry with the original | ||
// value from the KV store. | ||
// NOTE: if the insert a []byte{} value into the sqldb as the | ||
// entry value, and then retrieve it, the value will be | ||
// returned as nil. The bytes.Equal will pass in that case, | ||
// and therefore such cases won't error out. The kvdb instance | ||
// can store []byte{} values. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can the kv database currently have []byte(nil) stored via some code path? If
Should make sure to cover in a unit test
We add a helper function to the functions that creates the test SQL stores, in order to ensure that the store is properly closed when the test is cleaned up.
In the upcoming migration of the firewall database to SQL, the helper functions that creates the test databases of different types, need to return a unified interface in order to not have to control the migration tests file by build tags. Therefore, we export the unified interface FirewallDBs, so that it can be returned public test DB creation functions
In the upcoming migration of the firewall database to SQL, the helper functions that creates the test databases of different types, need to return a unified interface in order to not have to control the migration tests file by build tags. Therefore, we update the `NewTestDB` functions to return the `FirewallDBs` interface instead of the specific store implementation type.
During the upcoming upcoming migration of the firewall database to SQL, we need to be able to check all kvstores records in the SQL database, to validate that the migration is successful in tests. This commits adds a query to list all kvstores records, which enables that functionality.
Rename the session_id to group_id in kvstores table in the SQL store, to better represent how the field is actually used. Note that this is a breaking change, and would normally require a new migration. But as the SQL store is not used in production, and only enabled under the dev build flag, we can rename it without a new migration, as there's no users of the SQL store in production.
During the migration of the kvstores to SQL, we'll iterate over the buckets in the bbolt database, which holds all kvstores records. In order to understand why the migration iterates over the buckets in the specific order, we need to clarify the bbolt kvstores illustration docs, so that it correctly reflects how the records are actually stored in the bbolt database.
7bb668e
to
36e0f1c
Compare
This commit introduces the migration logic for transitioning the kvstores store from kvdb to SQL. Note that as of this commit, the migration is not yet triggered by any production code, i.e. only tests execute the migration logic.
36e0f1c
to
c3d8ecf
Compare
Thanks a lot for the in depth reviews @bitromortac & @ellemouton 🙏🔥! Addressed your feedback with the latest push. |
Based on #1051
This PR introduces the migration logic for transitioning the kv stores from kvdb to SQL.
Note that as of this PR, the migration is not yet triggered by any production code, i.e. only tests execute the migration logic.
Once #1051 is merged, I will rebase this PR and request reviews.
Part of #917